core/ConfigUtil: Do not normalize objectClass, component.name, component.id configuration values #4109

davidgraeff · 2017-08-22T14:12:16Z

The class ConfigUtil is used by Configuration to normalize values of a given Map<String,Object> configuration map.

I started to use Configuration recently also in OSGi services (in the activate and modified method), not only ThingHandlers, which works wonderfully. Except that I get a lot of warnings of the aforementioned class because it tries to normalize "objectClass" which is a complex object.

To be able to use the Configuration class like in the example below, the "objectClass" and two other OSGi default configuration parameters should be excluded.

    @Modified
    protected void modified(Map<String, Object> config) {
        configuration= new Configuration(config).as(NetworkBindingConfiguration.class);
    }

Signed-off-by: David Gräff david.graeff@web.de

sjsf · 2017-08-22T16:17:45Z

That clearly makes sense!

I'm just wondering whether this is the right/only place for this check... On the first glance there also is the normalizeTypes(Map<String, Object>, List<ConfigDescription>) which could suffer from the same bug, couldn't it?

In any case, I think putting this assertion into ConfigUtilTest to make sure it doesn't get broken by anybody accidentally would make sense.

davidgraeff · 2017-08-22T17:01:55Z

I cannot open projects with groovy code unfortunately, something is misconfigured in my eclipse. I converted one of the test suites to java but converting everything is a little tedious to just have this merged.

sjsf

I converted one of the test suites to java but converting everything is a little tedious

Fully ack, it's great like that, thanks! Better don't repair your IDE then... 😉

sjsf · 2017-08-23T06:37:38Z

...clipse.smarthome.config.core/src/main/java/org/eclipse/smarthome/config/core/ConfigUtil.java

@@ -138,8 +135,10 @@ public static Object normalizeType(Object value, ConfigDescriptionParameter conf
        for (Entry<String, ?> parameter : configuration.entrySet()) {
            String name = parameter.getKey();
            Object value = parameter.getValue();
-            ConfigDescriptionParameter configDescriptionParameter = configParams.get(name);
-            convertedConfiguration.put(name, normalizeType(value, configDescriptionParameter));
+            if (!name.equals("objectClass") && !name.equals("component.name") && !name.equals("component.id")) {


Now that I'm seeing this, I think it would be good to have a static set of strings defined once, then checking here (and above) only if the name is contained in there - just to make sure they don't run out of sync.

What about a private helper method instead?

sure, even better

…ent.id configuration values Signed-off-by: David Gräff <david.graeff@web.de>

triller-telekom · 2017-08-23T15:18:59Z

...clipse.smarthome.config.core/src/main/java/org/eclipse/smarthome/config/core/ConfigUtil.java

        for (Entry<String, Object> parameter : configuration.entrySet()) {
            String name = parameter.getKey();
            Object value = parameter.getValue();
-            convertedConfiguration.put(name, normalizeType(value));
+            if (!isOSGiConfigParameter(name)) {
+                convertedConfiguration.put(name, normalizeType(value));


You only add the value to your convertedConfiguration if it is not an OSGI parameter, is that by intention or is there an

else { convertedConfiguration.put(name, value); }

missing?

Yes that's the entire purpose of this PR, to ignore the OSGi parameters completely. They contain either dots so cannot be matched with a variable name or are complex objects, so cannot be marshaled to values.

+1 - these values don't make any sense for the service itself. It knows its name and pid.

Ah Ok, then the name of this PR was a bit misleading for me, because it soudned like you only want to omit the "normalization" and not the entire parameters. thanks for the clarification!

triller-telekom · 2017-08-23T15:20:06Z

...clipse.smarthome.config.core/src/main/java/org/eclipse/smarthome/config/core/ConfigUtil.java

-            convertedConfiguration.put(name, normalizeType(value, configDescriptionParameter));
+            if (!isOSGiConfigParameter(name)) {
+                ConfigDescriptionParameter configDescriptionParameter = configParams.get(name);
+                convertedConfiguration.put(name, normalizeType(value, configDescriptionParameter));


Same as above, don't you want to add an (un-normalized) OSGI parameter to your return map?

sjsf · 2017-08-24T06:17:27Z

Thanks!

davidgraeff force-pushed the configutil branch from 5d15fbd to 8de991d Compare August 22, 2017 16:59

sjsf reviewed Aug 23, 2017

View reviewed changes

core/ConfigUtil: Do not normalize objectClass, component.name, compon…

e9ea495

…ent.id configuration values Signed-off-by: David Gräff <david.graeff@web.de>

davidgraeff force-pushed the configutil branch from 8de991d to e9ea495 Compare August 23, 2017 14:16

davidgraeff mentioned this pull request Aug 23, 2017

Network: Fix native ping on MacOS, Fix arping with second arping utility openhab/openhab-addons#2580

Merged

triller-telekom reviewed Aug 23, 2017

View reviewed changes

sjsf merged commit e5b930a into eclipse-archived:master Aug 24, 2017

davidgraeff deleted the configutil branch August 24, 2017 06:34

kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/ConfigUtil: Do not normalize objectClass, component.name, component.id configuration values #4109

core/ConfigUtil: Do not normalize objectClass, component.name, component.id configuration values #4109

davidgraeff commented Aug 22, 2017

sjsf commented Aug 22, 2017

davidgraeff commented Aug 22, 2017

sjsf left a comment

sjsf Aug 23, 2017

davidgraeff Aug 23, 2017

sjsf Aug 23, 2017

davidgraeff Aug 23, 2017

triller-telekom Aug 23, 2017

davidgraeff Aug 23, 2017

sjsf Aug 24, 2017

triller-telekom Aug 24, 2017

triller-telekom Aug 23, 2017

sjsf commented Aug 24, 2017

core/ConfigUtil: Do not normalize objectClass, component.name, component.id configuration values #4109

core/ConfigUtil: Do not normalize objectClass, component.name, component.id configuration values #4109

Conversation

davidgraeff commented Aug 22, 2017

sjsf commented Aug 22, 2017

davidgraeff commented Aug 22, 2017

sjsf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjsf commented Aug 24, 2017